Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 1 file.
|
3bd6464 to
ea3cef2
Compare
There was a problem hiding this comment.
Pull request overview
Refactors the Charts package’s internal layout wrappers to use Stack from @wordpress/ui, replacing several ad-hoc flexbox <div> containers and removing now-redundant SCSS layout modifiers, while keeping visual layout behavior consistent and aligning spacing with design-system tokens.
Changes:
- Migrated legend container layout from BEM-based flexbox classes to
@wordpress/uiStack(keeping the root.legendselector hook). - Migrated key chart layouts to
Stack(conversion funnel wrappers, line-chart tooltip rows, annotation popover header, geo-chart container/placeholder). - Updated the donut chart Storybook custom legend example to use
Stackinstead of__experimentalHStack, and added a changelog fragment.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/charts/src/components/legend/private/base-legend.tsx | Replaces legend container <div> flex styling with Stack props (direction/gap/alignment/wrap). |
| projects/js-packages/charts/src/components/legend/private/base-legend.module.scss | Removes legend container layout modifier rules now handled by Stack. |
| projects/js-packages/charts/src/charts/pie-chart/stories/donut.stories.tsx | Replaces __experimentalHStack usage with Stack in the custom legend story. |
| projects/js-packages/charts/src/charts/line-chart/private/line-chart-annotation-label-popover.tsx | Converts annotation popover header layout wrapper to Stack. |
| projects/js-packages/charts/src/charts/line-chart/line-chart.tsx | Converts tooltip row wrapper to Stack for flex layout. |
| projects/js-packages/charts/src/charts/line-chart/line-chart.module.scss | Removes flex layout rules now provided by Stack in tooltip rows and popover header. |
| projects/js-packages/charts/src/charts/geo-chart/geo-chart.tsx | Centers loading and main geo chart render using Stack instead of flexbox CSS. |
| projects/js-packages/charts/src/charts/geo-chart/geo-chart.module.scss | Removes flex centering styles while preserving position: relative. |
| projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.tsx | Migrates main metric, funnel container, and step wrappers to Stack with tokenized gaps. |
| projects/js-packages/charts/src/charts/conversion-funnel-chart/conversion-funnel-chart.module.scss | Removes flex layout styles now handled by Stack. |
| projects/js-packages/charts/changelog/charts-175-use-stack-for-layout | Adds changelog fragment documenting the refactor. |
|
@cursoragent code review please |
|
Here's the code review for the PR, Adam. I found one thing worth fixing and went ahead and addressed it. The rest looks solid. Issue fixed (committed):
Everything else looks good:
|
@cursoragent these are gone, I think the branch may have been slightly out of date |
404ea39 to
30e86ee
Compare
4ee6195 to
0ed9f37
Compare
kangzj
left a comment
There was a problem hiding this comment.
Really nice refactor — this is exactly the kind of cleanup AGENTS.md was pointing at, and the per-surface Storybook testing notes made the diff very easy to follow. The deviation notes on TrendIndicator and the 2px step-label were a great touch. Left a few small, non-blocking suggestions inline — take or leave them, nothing blocking this.
| { matchedItem.value } | ||
| </span> | ||
| ) } | ||
| <Stack align="center" gap="sm" justify={ labelJustifyContent }> |
There was a problem hiding this comment.
Tiny nit — one subtle semantics shift worth a line comment: labelJustifyContent previously lived on LegendLabel, which only had one child (.legend-item-label), so passing 'space-between' was effectively a no-op. Now it's on a <Stack> with two children (text + value), so 'space-between' will actually push them apart. Grepped the repo and no current caller relies on the old behavior (only maxWidth / textOverflow are used externally), so no real risk — but a one-liner noting that this prop now governs intra-label spacing between text and value would save a future reader from a double-take. Totally optional.
There was a problem hiding this comment.
I don't think this is correct. LegendLabel was wrapping both LegendText and the legend-item-value before, and this is still the case after with the Stack that has been introduced. The style has been transferred to that element, so the behaviour should be maintained.
| <div> | ||
| { renderStepLabel ? ( | ||
| renderStepLabel( { | ||
| step, |
There was a problem hiding this comment.
Micro-cleanup suggestion: this <div> used to carry styles['step-header'] purely for the margin-bottom: 24px that's now handled by the parent Stack's gap="xl". With no className and no other purpose, it could probably collapse into a <Fragment> (or disappear entirely) to shave one DOM node. Very minor — keep it if you prefer the explicit grouping.
| className={ clsx( 'geo-chart', styles.container, className ) } | ||
| data-testid="geo-chart-loading" | ||
| style={ { width, height } } | ||
| > |
There was a problem hiding this comment.
Both the loading placeholder and the main container use <Stack align="center" justify="center"> around a single child — so Stack here is really acting as a centered flex wrapper rather than a layout primitive. Not wrong at all, just worth noting for consistency: a plain <div> with centering CSS (or a dedicated centering primitive if one exists) might be a more honest signal of intent. Happy to defer to whatever keeps the package consistent.
There was a problem hiding this comment.
Yeah I think this is still a valid use of a Stack despite the name. I think a more appropriate name for the Stack component might be Flex, as that's what it provides essentially. That said, a centering component abstraction could be useful, I'll take a look at what sort of usage this would have.
There was a problem hiding this comment.
Claude's take: a thin <Center> primitive is warranted, but only barely. Five sites is around the threshold where an abstraction starts paying for itself, and two of them already hand-named the centering intent via *__centering CSS classes — that's a tell that "centered box" is a recurring concept, not incidental flexing. A
- Communicate intent (kangzj's point — Stack-as-centering reads awkwardly)
- Let the pie charts drop their
*__centeringclass names - Be ~10 lines wrapping Stack with fixed
align="center" justify="center"defaults
Tradeoff: one more primitive to learn, and the implementation is trivially a Stack underneath. If you're renaming Stack → Flex anyway (per your reply), consider doing both together — <Flex> for general flex layout, <Center> for the centering case. Separate follow-up PR, not this one.
My take: I didn't mean we'd actually rename Stack to Flex. I think a Center component does make sense given we have existing styles doing this. I'll create a separate issue.
There was a problem hiding this comment.
dognose24
left a comment
There was a problem hiding this comment.
Works as expected from my testing! 👍🏼
| <div> | ||
| { renderStepLabel ? ( | ||
| renderStepLabel( { | ||
| step, |
| // Default tooltip rendering function | ||
| const renderDefaultTooltip = ( step: FunnelStep ) => ( | ||
| <> | ||
| <Stack direction="column" align="flex-start" gap="xs"> |
| > | ||
| { labels => ( | ||
| <div | ||
| <Stack |
| </div> | ||
| { tooltipPoints.map( point => ( | ||
| <div key={ point.key } className={ styles[ 'line-chart__tooltip-row' ] }> | ||
| <Stack |
| data-testid="line-chart-annotation-label-popover" | ||
| > | ||
| <div className={ styles[ 'line-chart__annotation-label-popover-header' ] }> | ||
| <Stack direction="row" align="flex-start" justify="space-between"> |
|
|
||
| return ( | ||
| <div | ||
| <Stack |
|
|
||
| { /* Funnel Steps */ } | ||
| <div className={ styles[ 'funnel-container' ] }> | ||
| <Stack direction="row" align="flex-end" gap="lg" className={ styles[ 'funnel-container' ] }> |
Replaces the legend container <div> + ad-hoc flex SCSS modifier classes with a <Stack> from @wordpress/ui. Gap values move from hardcoded px (8px/16px) to design-system tokens (sm/lg). Root .legend class is preserved as the component's identifying selector hook. Refs CHARTS-175.
Replaces .main-metric, .funnel-container, and .funnel-step ad-hoc flex divs with <Stack> from @wordpress/ui. Gap values move to design-system tokens (sm=8px, lg=16px). The .tooltip-wrapper stays as-is because it is rendered inside @visx/tooltip's TooltipInPortal, which controls its own DOM and positioning. Refs CHARTS-175.
Replaces the line-chart tooltip row and annotation popover header divs with <Stack> from @wordpress/ui. The trigger-button centering stays as SCSS — it's button-internal positioning, not layout. Refs CHARTS-175.
Both the loading placeholder and the main chart container are now <Stack> elements with centered alignment. position: relative stays in SCSS because Google Charts' GeoChart needs it for tooltip positioning. Refs CHARTS-175.
Migrates the CustomPieLegend example off @wordpress/components' experimental HStack onto the stable Stack from @wordpress/ui. Refs CHARTS-175.
The line-chart__annotation-label-popover-header class had no corresponding SCSS rule and was left over from the migration to Stack. Drop it.
The .tooltip-wrapper class was carrying both layout (inline-flex, column, gap: 4px) and visual chrome (border, padding, box-shadow), and it's applied to visx's TooltipInPortal wrapper. Previously I skipped this in 8501724 because the portal controls its own DOM, but the children inside it are ours — so wrap the default tooltip children in a <Stack> and keep only the visual styling on the portal wrapper. Custom renderTooltip callers are responsible for their own layout; the className they receive now provides only visual chrome, which matches the intent of a "custom" tooltip override. Refs CHARTS-175.
Replace the `.bar-container` button div with a `<Stack>` carrying the existing button semantics (onClick/onKeyDown/role/tabIndex/ aria-label). The remaining `.bar-container` SCSS keeps `flex: 1`, border-radius, position, and cursor — visual styling that doesn't fit Stack props. Also drop `.step-header` and lift its 24px bottom margin onto the parent funnel-step Stack as `gap="xl"`, which is the layout-token equivalent and removes a CSS rule we no longer need. Refs CHARTS-175.
The .main-metric class carried a margin-bottom: 24px to space it from the .funnel-container below. Now that both are siblings inside the outer column Stack, that 24px belongs on the parent as gap="xl" — the design-system token equivalent — so drop the SCSS margin and add the gap prop. Refs CHARTS-175.
The .legend-item-label SCSS rule carried display: flex, align-items: center, and gap: 0.5rem to lay out the label text + optional value span inside visx's LegendLabel. Wrap those children in a <Stack> with gap="sm" (8px) and drop the SCSS rule. The .legend-item-label class stays on LegendLabel as a selector hook for the .legend-item--inactive .legend-item-label line-through rule. The labelJustifyContent prop moves from LegendLabel's inline style onto the Stack, since the Stack is now the actual flex container. Refs CHARTS-175.
visx's LegendItem applies display: flex, align-items: center, flex-direction, and margin as inline styles by default (see @visx/legend's LegendItem.js), so the matching SCSS rules in .legend-item were dead code overridden by the inline styles. Delete them. Refs CHARTS-175.
The mapping is static and doesn't depend on props or state. Moving it out of the render body avoids recreating the object on every render.
0ed9f37 to
58b79c3
Compare









Fixes CHARTS-175
Proposed changes
<div>with BEM modifier classes (.legend--horizontal,.legend--vertical,.legend--alignment-*) to a<Stack>from@wordpress/ui. The root.legendclass is preserved as an identifying selector hook; layout-only modifier classes are dropped.LegendText+ value span) to a<Stack gap="sm">, replacing thedisplay: flex; align-items: center; gap: 0.5remSCSS rule. Drop the redundantdisplay: flex; align-items: centerrules from.legend-item— visx'sLegendItemapplies those as inline styles by default, so the SCSS was dead code..main-metric,.funnel-container,.funnel-step,.bar-container) to<Stack>. The.bar-containerkeeps its button semantics (onClick/onKeyDown/role/tabIndex/aria-label) on the Stack wrapper..main-metric { margin-bottom: 24px }becomesgap="xl"on the outer funnel Stack, and.step-header { margin-bottom: 24px }(deleted entirely) becomesgap="xl"on the funnel-step Stack.<Stack>. The.tooltip-wrapperclass now carries only visual chrome (border, padding, box-shadow); its layout properties move onto a<Stack>wrapping the tooltip children insideTooltipInPortal. CustomrenderTooltipcallers are responsible for their own layout.<Stack>. Button-internal icon centering (.annotation-label-trigger-button,.annotation-label-popover-close-button) stays as SCSS — it's single-icon button centering, not layout.<Stack>with centered alignment.position: relativestays in SCSS because Google Charts' GeoChart relies on it for tooltip positioning.__experimentalHStackfrom@wordpress/componentswithStackfrom@wordpress/uiin the donut story'sCustomPieLegendexample.4px → xs,8px → sm,16px → lg,24px → xl. This contributes to CHARTS-199 (spacing/border tokenization).Other information
Two deviations from the original issue scope, both documented in the commit messages:
TrendIndicatoris a publicly exported component rendered as<span display: inline-flex>.Stackfrom@wordpress/uionly renders<div>, which would change the element from inline to block — a backward-compat regression risk for consumers using it in flowing text. A follow-up is trivial if/whenStackgains anasprop..step-label { margin: 0 0 2px 0 }couldn't be lifted to a Stackgapbecause@wordpress/uiStack only accepts design-system tokens (xs=4pxis the smallest), not raw pixel values. Migrating would have rounded 2px → 4px, so the margin stays as-is.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
This is a refactor with no intended behavior changes. Verification is existing tests + visual parity in Storybook.
pnpm --filter @automattic/charts test— expect 841 passing tests across 43 suites.pnpm --filter @automattic/charts typecheck— expect clean.cd projects/js-packages/storybook && pnpm run storybook:devand navigate to each migrated surface. Layout should be visually identical to trunk:JS Packages / Charts Library / Components / Legend → Default(horizontal, items side-by-side with 16px gap) andVertical(items stacked with 8px gap, center-aligned). Items with values should show 8px gap between label text and value.JS Packages / Charts Library / Charts / Conversion Funnel Chart → Default. Main metric (10.3% +2%) should be baseline-aligned with 8px between values, and 24px below the funnel. Four steps (Sessions/Cart/Checkout/Purchase) should have 16px gap, bars bottom-aligned, and 24px between each step's label/rate header and its bar. Click a funnel bar — the default tooltip should show title + rate/items with 4px gap between them.JS Packages / Charts Library / Charts / Line Chart → Default. Hover the chart — tooltip rows should have label left, value right, space-between layout.JS Packages / Charts Library / Charts / Line Chart / Annotations → Default. Three annotation labels should render with title/subtitle column layout.JS Packages / Charts Library / Charts / Geo Chart → Default. World map should be centered in the container; tooltips on country hover should still position correctly.JS Packages / Charts Library / Charts / Donut Chart → Custom Legend. Legend rows should show color dot + label (8px gap) + right-aligned values and comparison percentages.Note
This unrelated issue was discovered while testing: CHARTS-208